Fix type narrowing for variable swaps after isinstance checks#21624
Fix type narrowing for variable swaps after isinstance checks#21624BHUVANSH855 wants to merge 2 commits into
Conversation
|
Diff from mypy_primer, showing the effect of this PR on open source code: discord.py (https://github.com/Rapptz/discord.py)
- discord/app_commands/commands.py:1583: error: Incompatible types in assignment (expression has type "str", variable has type "locale_str | None") [assignment]
- discord/app_commands/commands.py:1595: error: Incompatible types in assignment (expression has type "str", variable has type "locale_str | None") [assignment]
|
|
I investigated the mypy-primer report from discord.py. The reported locations are tuple assignments of the form: which are directly affected by this change. I tried reproducing the pattern locally with a minimal example and mypy reports no issues: All existing mypy tests also pass, including the full testcheck suite. I'd appreciate maintainer feedback on whether the primer diffs represent genuine newly-detected issues or indicate a problem with the implementation. |
adhavan18
left a comment
adhavan18
left a comment
There was a problem hiding this comment.
clean fix for a real simultaneous-assignment narrowing bug. a few thoughts:
the approach is correct — capturing narrowed RHS types before processing any pair is exactly the right way to implement true simultaneous-assignment semantics. the binder mutation during earlier pairs was the root cause and this sidesteps it neatly.
one edge case worth checking: self.binder.get(rv) returns None when the name has no narrowed type in the current binder frame. the fallthrough to captured_pairs.append((lv, rv)) handles that correctly. but if narrowed is a DeletedType (e.g., the variable was conditionally deleted), temp_node will produce a node with DeletedType — does check_assignment handle that gracefully, or should there be a not isinstance(narrowed, DeletedType) guard before capturing?
test coverage looks solid — the direct swap case (a, b = b, a) and the tuple intermediary case cover the two main patterns. might be worth adding a case where one variable is NOT narrowed to confirm only the narrowed side is captured (the current fallthrough path).
overall the change is minimal and well-targeted. the captured_pairs two-pass pattern is easy to follow.
2 participants
Fixes #21586
Summary
This PR fixes incorrect type narrowing when variables are swapped after an
isinstance()check.Previously, in code such as:
mypy incorrectly revealed
basSub1instead ofSub2.The issue occurred because narrowed binder information for RHS variables could be overwritten while processing a multi-assignment statement. This change captures narrowed RHS
NameExprtypes before assignments are applied, preserving simultaneous assignment semantics.Changes
Capture narrowed RHS variable types before processing multi-assignment pairs.
Use the captured types when checking assignments to avoid interference from earlier assignments in the same statement.
Add regression tests covering:
isinstance()narrowing.Testing
Verified with:
python -m pytest mypy/test/testcheck.py -k testIsInstanceVariableSwapNarrowing -n0python -m pytest mypy/test/testcheck.py -k testTupleSwapAfterIsinstance -n0python -m pytest mypy/test/testcheck.py -k narrowing -n0python -m pytest mypy/test/testcheck.py -k assignment -n0python -m pytest mypy/test/testcheck.py -k tuple -n0python -m pytest mypy/test/testcheck.py -k isinstance -n0python -m pytest mypy/test/testcheck.py -n0python -m pytest mypy/test/testcheck.py -n autoAll tests pass.